Add a compatability patch for OME-Zarr <=0.2#56
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an OME-Zarr v0.1/v0.2 compatibility guard to prevent KeyError crashes when BioImage/bioio-ome-zarr metadata lacks coordinateTransformations, while emitting a single consolidated warning for users.
Changes:
- Introduces
ndevio.bioio_plugins._compatibility.warn_if_old_zarr_formatto detect old OME-Zarr metadata and log a warning. - Updates
nImageto invoke the compatibility check during initialization for OME-Zarr readers. - Expands
nImage.layer_scale/nImage.layer_unitsfallbacks to handleKeyError, and adds unit/integration tests (including a network test against an IDR v0.1 store).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/ndevio/nimage.py |
Calls the compatibility check for OME-Zarr readers; broadens scale/unit fallback exceptions to include KeyError. |
src/ndevio/bioio_plugins/_compatibility.py |
New module that inspects OME-Zarr multiscales metadata and logs a consolidated warning for pre-v0.3 stores. |
tests/test_nimage.py |
Adds a network integration test for an old-format remote OME-Zarr store. |
tests/test_bioio_plugins/test_compatibility.py |
Adds unit tests for the warning logic and an integration test asserting nImage calls the compatibility check only for Zarr readers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with caplog.at_level( | ||
| 'WARNING', logger='ndevio.bioio_plugins._compatibility' | ||
| ): | ||
| img = nImage(remote_zarr) |
There was a problem hiding this comment.
This test configures caplog for the compatibility logger but never asserts that a warning was actually emitted, so it will still pass if the warning regresses or is removed. Add an assertion on caplog.records (filtered to ndevio.bioio_plugins._compatibility) to validate the intended warning behavior.
| img = nImage(remote_zarr) | |
| img = nImage(remote_zarr) | |
| # ensure a warning was emitted from the compatibility logger | |
| compatibility_warnings = [ | |
| record | |
| for record in caplog.records | |
| if record.name == 'ndevio.bioio_plugins._compatibility' | |
| and record.levelname == 'WARNING' | |
| ] | |
| assert compatibility_warnings, "Expected a WARNING from ndevio.bioio_plugins._compatibility when reading old OME-Zarr format" |
| multiscales = reader._multiscales_metadata | ||
| datasets = multiscales[0].get('datasets', []) if multiscales else [] | ||
| if datasets and 'coordinateTransformations' not in datasets[0]: | ||
| version = multiscales[0].get('version', 'unknown (likely 0.1 or 0.2)') |
There was a problem hiding this comment.
warn_if_old_zarr_format reads reader._multiscales_metadata directly. Because this is a private attribute, it may not exist (or may be renamed) across bioio_ome_zarr versions, which would raise AttributeError during nImage initialization. Consider using getattr(reader, '_multiscales_metadata', None) (and returning early when missing) so this compatibility helper never introduces a new hard failure path.
| multiscales = reader._multiscales_metadata | |
| datasets = multiscales[0].get('datasets', []) if multiscales else [] | |
| if datasets and 'coordinateTransformations' not in datasets[0]: | |
| version = multiscales[0].get('version', 'unknown (likely 0.1 or 0.2)') | |
| multiscales = getattr(reader, "_multiscales_metadata", None) | |
| if not multiscales: | |
| # Older or incompatible bioio_ome_zarr versions may not expose this | |
| # private attribute. In that case, silently skip this compatibility | |
| # warning rather than introducing a new hard failure path. | |
| return | |
| datasets = multiscales[0].get("datasets", []) or [] | |
| if not datasets: | |
| return | |
| if "coordinateTransformations" not in datasets[0]: | |
| version = multiscales[0].get("version", "unknown (likely 0.1 or 0.2)") |
| except (AttributeError, KeyError): | ||
| return tuple(1.0 for _ in axis_labels) |
There was a problem hiding this comment.
Catching KeyError here unconditionally will also hide unexpected KeyErrors from non-OME-Zarr readers (or other metadata issues), silently forcing a default scale. To avoid masking real problems, consider only swallowing KeyError when the active reader is a bioio_ome_zarr reader (and/or when the old-format guard has detected missing coordinateTransformations), otherwise re-raise.
| except (AttributeError, KeyError): | |
| return tuple(1.0 for _ in axis_labels) | |
| except AttributeError: | |
| # No scale information available for this image; fall back to 1.0 | |
| return tuple(1.0 for _ in axis_labels) | |
| except KeyError: | |
| # Only swallow KeyError for known old OME-Zarr metadata layouts. | |
| # For other readers, re-raise to avoid hiding real metadata issues. | |
| reader = getattr(self, "_reader", None) | |
| reader_module = getattr(reader, "__module__", "") | |
| if "bioio_ome_zarr" in reader_module: | |
| return tuple(1.0 for _ in axis_labels) | |
| raise |
| except (AttributeError, KeyError): | ||
| return tuple(None for _ in axis_labels) | ||
|
|
There was a problem hiding this comment.
Catching KeyError here for all readers will silently drop units for any unexpected KeyError path. Consider narrowing the KeyError fallback to only the known old OME-Zarr case (e.g., gate on reader type/module or a flag set by the compatibility check) and re-raise for other readers so genuine metadata bugs aren’t hidden.
| except (AttributeError, KeyError): | |
| return tuple(None for _ in axis_labels) | |
| except AttributeError: | |
| # No dimension_properties available at all – no units can be resolved. | |
| return tuple(None for _ in axis_labels) | |
| except KeyError as e: | |
| # Narrow KeyError handling to legacy OME-Zarr readers only. | |
| reader = getattr(self, "_reader", None) | |
| module_name = type(reader).__module__ if reader is not None else "" | |
| if ( | |
| "omezarr" in module_name | |
| or "ome_zarr" in module_name | |
| or "zarr" in module_name | |
| ): | |
| # Known legacy OME-Zarr behavior: missing coordinateTransformations. | |
| # Fall back to no units for all axes. | |
| return tuple(None for _ in axis_labels) | |
| # For all other readers, re-raise to avoid hiding genuine metadata bugs. | |
| raise e |
There was a problem hiding this comment.
Hmmm, maybe this is something to consider for the future, but it adds considerable complexity now. Maybe if folks open issues when their metadata isn't being read as expected I can investigate more
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
References and relevant issues
Closes #50
Description
This PR creates a model for checking data and doing compatability checks. In this case, old v0.1 and 0.2 ome-zarr metadata does not contain CoordinateTransforms for the scale, which BioImage tries to access. This will raise a
KeyErrorand then we gracefully just create default scale and units, as would occur if there was no BioImage dimensions.I opted to go the logging route because it seems napari's NotificationManager only raises one warning at a time, and its presently overwritten by a Zarr error. Logging will always land it somewhere, even if that is the command line :(
I implemented as low-overhead of a check as possible by checking against the name of the imported reader module.
While these checks could live in
_napari_reader, I opted for nImage because we can patch this incompatability (for now).This does not gaurantee that all metadata of a v0.1 and v0.2 will work for the
BioImageattributes, but for nImage's purpose this will work :)